feat(client): retry transient GET failures with backoff#37
Conversation
| response: requests.Response | None = None | ||
| for attempt in range(self._MAX_RATE_LIMIT_RETRIES + 1): | ||
| response = self._request_with_same_origin_redirects( | ||
| response = self._request_with_transient_retries( |
There was a problem hiding this comment.
(AI-assisted)
Could we enforce the advertised seven-attempt budget across the composed retry layers?
429 is correctly excluded from _RETRYABLE_GET_STATUSES, but each outer rate-limit retry calls _request_with_transient_retries() again with a fresh budget. A sequence of six 503s followed by 429, repeated across the four rate-limit rounds, therefore makes 28 physical requests for one logical GET; the 401 refresh path can repeat the composition again.
Could we use one shared attempt/deadline budget across transient, rate-limit, and auth-refresh retries, and add a mixed 503/429 regression test that asserts the total request count? That would avoid retry amplification during an outage while preserving Retry-After handling.
429fe5a to
a575da1
Compare
Idempotent GETs now retry on 500/502/503/504 and transient transport errors (connection reset/refused, timeout, truncated body) with doubling backoff capped at 30s, up to 7 attempts per logical request. Mutating methods are never duplicated and streamed GETs are excluded so stream consumers keep control of the open path. SSL certificate errors raise on the first attempt because they are deterministic and retrying only delays the report. The existing 429 Retry-After handling is unchanged and composes with the new layer. Retry warnings go through the client logger; pipeline-run, artifact, pipeline hydration, published-component, and secret commands thread their --log-type logger into the client so the warnings follow the configured sink, and logger-less programmatic clients stay silent.
a575da1 to
3f3421b
Compare
| if response.status_code != 429 or not budget.can_retry(): | ||
| return response | ||
| self._sleep_for_rate_limit(response, attempt) | ||
| return response | ||
| self._sleep_for_rate_limit(response, rate_limit_round) | ||
| rate_limit_round += 1 |
There was a problem hiding this comment.
(AI-assisted)
Could we re-check the retry deadline after sleeping, before issuing the next request? can_retry() is evaluated before _sleep_for_rate_limit(), so a long Retry-After can advance past the shared deadline and the loop still performs another unconditional send. In a deterministic reproduction, a 10-second deadline plus Retry-After: 60 sent once at t=0 and again at t=60; the transient-backoff path has the same pattern. Please cap the sleep to the remaining budget and/or atomically check/consume the budget immediately before every physical send.
| budget.consume() | ||
| attempt += 1 | ||
| try: | ||
| response = self._request_with_same_origin_redirects( |
There was a problem hiding this comment.
(AI-assisted)
Could we charge the shared retry budget for every actual session.request(), including redirects? This budget.consume() runs once before _request_with_same_origin_redirects(), but that helper may perform several physical sends. A deterministic 307 → 503 sequence repeated for seven retry rounds produced 14 sends while consuming only seven units; five redirects per round can reach 42 sends. Please move the budget check/consume to the physical-send boundary so each redirect also counts.
| if method.upper() != "GET" or request_kwargs.get("stream"): | ||
| budget.consume() |
There was a problem hiding this comment.
(AI-assisted)
Nit: could we preserve the legacy four-attempt 429 cap for POSTs and streamed GETs? These requests correctly bypass transient retries, but they still consume the shared seven-unit budget in the outer 429 loop. Ten queued 429s therefore produce seven sends instead of the previous four, and the default no-header backoff grows from 7 seconds total to 63. Since the PR says existing 429 behavior is preserved, either retain the smaller cap for these request classes or explicitly document and test the change.
2 participants
Summary
requests.exceptions.SSLErroron the first attempt: certificate failures are deterministic, so retrying only delays the report--log-typelogger to the client, while logger-less programmatic clients remain silentContext
Short backend restarts and connection resets currently fail read-only CLI operations immediately even though repeating the request is safe. This adds bounded recovery for GET requests without risking duplicate writes or changing rate-limit behavior, and keeps every retry visible through the logger each CLI command already configures.
Testing
uv run pytest tests/test_client.py tests/test_pipeline_runs_cli.py tests/test_artifacts_cli.py tests/test_components_cli.py tests/test_secrets_cli.pyuvx ruff checkon the touched source filesuv lock --checkgit diff --check